Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Add special case for Homebrew #6

Merged
merged 2 commits into from
Apr 30, 2019
Merged

Add special case for Homebrew #6

merged 2 commits into from
Apr 30, 2019

Conversation

boneskull
Copy link
Contributor

After poking at this a bit further, I found we'd need extra I/O to make this work in every case.

If, for example, your executable is ava, we can't use process.env._ because it points to /path/to/ava. If we instead ran node node_modules/.bin/ava, then process.env._ would point to node in your PATH.

I've updated the code to check if node is indeed what the user executed; if that's the case, we can use process.env._ to get at its path. Otherwise, we just use process.execPath. On my system, this is still incorrect when running ava.

If we wanted this to always work, we'd need to use which or something to that effect, and I assume you don't want to be invoking child_process.anything() here.

@boneskull
Copy link
Contributor Author

Looks like the tests won't run on Node.js v4. I can add a commit to drop v4 from .travis.yml (and update engines field, add new Node.js versions to CI, etc)

@boneskull
Copy link
Contributor Author

another approach would just be to just look for /usr/local/Cellar/node/ in process.execPath and force it to use /usr/local as the prefix.

@boneskull
Copy link
Contributor Author

@sindresorhus (ping) I could use some direction here; there's a couple ways to go and I'm not sure which you'd prefer. The choices are:

  1. Consume which or something similar
  2. Add a special case for Homebrew

@vladimyr vladimyr mentioned this pull request Jan 13, 2019
Closed
@LitoMore
Copy link

LitoMore commented Jan 23, 2019 •
edited
Loading

@boneskull @sindresorhus I think we could drop support for Node.js 4. Just do it. Sindre already.

Sindre has removed support for Node.js 4 in many of his repositories.

@sindresorhus
Copy link
Owner

I would prefer 1, adding a special case for Homebrew.

@boneskull boneskull force-pushed the symlink-fix branch 5 times, most recently from 920857b to 7f83822 Compare April 26, 2019 06:36
@boneskull boneskull changed the title prefer process.env._ over process.execPath if the former points to node; closes #5 add special case for Homebrew Apr 26, 2019
@boneskull
Copy link
Contributor Author

@sindresorhus OK, this should be ready.

I tried to add OSX to the build matrix and install node using homebrew, but was running in to trouble and don't have the time to hack on it further atm.

Before this change:

$ npm test

> [email protected] test /Users/boneskull/projects/sindresorhus/global-dirs
> xo && ava

{ npm:
   { prefix: '/usr/local/Cellar/node/11.14.0_1',
     packages: '/usr/local/Cellar/node/11.14.0_1/lib/node_modules',
     binaries: '/usr/local/Cellar/node/11.14.0_1/bin' },
  yarn:
   { prefix: '/Users/boneskull/.config/yarn',
     packages: '/Users/boneskull/.config/yarn/global/node_modules',
     binaries: '/Users/boneskull/.config/yarn/global/node_modules/.bin' } }

  1 passed
  3 failed

  npm.prefix

  /Users/boneskull/projects/sindresorhus/global-dirs/test.js:10

    9: test('npm.prefix', async t => {
   10:   t.is(globalDirs.npm.prefix, await npm(['prefix', '--global']));
   11: });

  Difference:

  - '/usr/local/Cellar/node/11.14.0_1'
  + '/usr/local'



  npm.packages

  /Users/boneskull/projects/sindresorhus/global-dirs/test.js:14

   13: test('npm.packages', async t => {
   14:   t.is(globalDirs.npm.packages, await npm(['root', '--global']));
   15: });

  Difference:

  - '/usr/local/Cellar/node/11.14.0_1/lib/node_modules'
  + '/usr/local/lib/node_modules'



  npm.binaries

  /Users/boneskull/projects/sindresorhus/global-dirs/test.js:18

   17: test('npm.binaries', async t => {
   18:   t.is(globalDirs.npm.binaries, await npm(['bin', '--global']));
   19: });

  Difference:

  - '/usr/local/Cellar/node/11.14.0_1/bin'
  + '/usr/local/bin'

After this change:

$ npm test

> [email protected] test /Users/boneskull/projects/sindresorhus/global-dirs
> xo && ava

{ npm:
   { prefix: '/usr/local',
     packages: '/usr/local/lib/node_modules',
     binaries: '/usr/local/bin' },
  yarn:
   { prefix: '/Users/boneskull/.config/yarn',
     packages: '/Users/boneskull/.config/yarn/global/node_modules',
     binaries: '/Users/boneskull/.config/yarn/global/node_modules/.bin' } }

  4 passed

@boneskull
Copy link
Contributor Author

Once this is released, I'll update create-yo to use this module.

@sindresorhus sindresorhus changed the title add special case for Homebrew Add special case for Homebrew Apr 30, 2019
@sindresorhus sindresorhus merged commit ddee7ea into sindresorhus:master Apr 30, 2019
@sindresorhus
Copy link
Owner

I'll do a new release once the other open PRs are merged.

@boneskull boneskull deleted the symlink-fix branch April 30, 2019 18:35
@byCedric byCedric mentioned this pull request Apr 30, 2019
Open
2 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@boneskull @LitoMore @sindresorhus